-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add TestResultsFlow for measuring time to notification #439
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: tasks/upload.py
Did you find this useful? React with a 👍 or 👎 |
3d33289
to
7638bed
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.46% 97.46%
=======================================
Files 417 417
Lines 34796 34819 +23
=======================================
+ Hits 33913 33936 +23
Misses 883 883
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.46% 97.46%
=======================================
Files 417 417
Lines 34796 34819 +23
=======================================
+ Hits 33913 33936 +23
Misses 883 883
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.46% 97.46%
=======================================
Files 417 417
Lines 34796 34819 +23
=======================================
+ Hits 33913 33936 +23
Misses 883 883
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.48% 97.48%
=======================================
Files 448 448
Lines 35525 35548 +23
=======================================
+ Hits 34632 34655 +23
Misses 893 893
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes This change has been scanned for critical changes. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be interesting to add another checkpoint in the beginning of TestResultsFinisherTask.process_impl_within_lock
like TEST_RESULTS_PROCESSING_BEGIN
.
THe subflow TEST_RESULTS_BEGIN --> TEST_RESULTS_FINISHER_BEGIN
would give us a rough estimate of processing time. (I think that the finisher task only runs after all the processor tasks finish, and that we don't current have a metric for that)
What do you think?
But in general lgtm 👍
Very exciting to see more Flows
helpers/checkpoint_logger/flows.py
Outdated
@success_events("TEST_RESULTS_BEGIN") | ||
@subflows( | ||
("notification_latency", "TEST_RESULTS_BEGIN", "TEST_RESULTS_NOTIFY"), | ||
("notification_latency", "TEST_RESULTS_BEGIN", "FLAKE_DETECTION_NOTIFY"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the subflows have the same name?
7638bed
to
20ec9ae
Compare
20ec9ae
to
ba595f7
Compare
helpers/checkpoint_logger/flows.py
Outdated
@failure_events("TEST_RESULTS_ERROR") | ||
@success_events("TEST_RESULTS_BEGIN") | ||
@subflows( | ||
("notification_latency", "TEST_RESULTS_BEGIN", "TEST_RESULTS_NOTIFY"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between "notification_latency" and "test_results_notification_latency" subflows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was a mistake :P (fixed now)
Signed-off-by: joseph-sentry <[email protected]>
082ea46
to
adc9560
Compare
No description provided.